Skip to content

Conversation

compiler-errors
Copy link
Member

We were previously lowering RPITITs to associated items with the name "kw::Empty", which as petrochenkov predicted, causes name collisions when being decoded from foreign crates.

Instead, synthesize a name by concatenating the def-path segments of the original RPITIT, something like foo::{opaque#1} or foo::{opaque#0}::{opaque#0}. This should keep the item unnameable, since we never want to name it as that would be problematic as we lower these items after the resolver is run.

Also, fix a few unrelated bugs with nested RPITITs, and beefen up the foreign encoding test.

cc @petrochenkov and the test case described here: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/-Z.20lower-impl-trait-in-trait-to-assoc-ty.20summary.3F/near/343451927

@rustbot
Copy link
Collaborator

rustbot commented Mar 21, 2023

r? @Nilstrieb

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 21, 2023
@petrochenkov petrochenkov self-assigned this Mar 21, 2023
@@ -281,7 +281,7 @@ pub enum DefPathData {
/// An `impl Trait` type node.
ImplTrait,
/// `impl Trait` generated associated type node.
ImplTraitAssocTy,
ImplTraitAssocTy(Symbol),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there's no real reason why this needs to be a separate def path data. We could use TypeNs here now, (I guess) as long as we keep the actual name that the RPITIT is lowering to unnameable.

@rust-cloud-vms rust-cloud-vms bot force-pushed the give-rpitits-real-names branch from 4624429 to 492edeb Compare March 21, 2023 18:56
@spastorino
Copy link
Member

spastorino commented Mar 21, 2023

Btw, it looks good to me but I'd leave this up to @petrochenkov given that they've self-assigned it.

@bors

This comment was marked as resolved.

@rust-cloud-vms rust-cloud-vms bot force-pushed the give-rpitits-real-names branch from 492edeb to 602cf24 Compare March 21, 2023 23:45
@petrochenkov
Copy link
Contributor

I think this is a big unnecessary complication.
See how macro_rules are ignored in build_reduced_graph_for_external_crate_res despite being in the children list.
Synthetic RPITITs can be ignored in the same way.

@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 22, 2023

It's kind of a philosophical question what exactly should go into the children list.

Right now only named items that are actually planted into modules and visible to other modules/crates in this way are put into the children list, that it's main use case.

Also macro_rules are put into the same list. Why? For historical reasons. They have to be ignored in the main user of children (build_reduced_graph_for_external_crate_res ) and could easily go into a separate list. I actually planned to remove them from this list, just never got to it.

Are synthetic RPITITs similar to macro_rules in this case?
Yes, they are not named and not planted into trait "modules ", so they either need to go to a separate list, or be ignored by the main user of the children list.
Something like that.

(I think I have a weak preference for separate lists for both after all.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2023
@spastorino
Copy link
Member

I don't think this is really complicated, the meat of the change is 602cf24 and the change also help to make diagnostics be equal to master as seen in https://github.com/rust-lang/rust/pull/109455/files#diff-2245ba262b8ba2866c27f1ac3f76db96146870beb91a6ac7ce621ca2cbb7c501R9. Although I guess that could also be Trait::{opaque} instead of Trait::method::{opaque} and it would also be fine.

@bors
Copy link
Collaborator

bors commented Mar 22, 2023

☔ The latest upstream changes (presumably #109496) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2023
@petrochenkov
Copy link
Contributor

It's not very complicated, it's just unnecessarily complicated (I'm only talking about the last commit, others seem unrelated).
If these types don't have a name then they shouldn't have a name.
Then you also won't have to invent some hygiene emulation scheme to synthesize names (which currently breaks if two RPITITs are defined by the same method?).

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2023
@spastorino
Copy link
Member

(which currently breaks if two RPITITs are defined by the same method?).

I don't think is going to break, you would end with Trait::method::{opaque#0}, Trait::method::{opaque#1} and so on.

Anyway, would leave this up to be answered by @compiler-errors

@Noratrieb Noratrieb removed their assignment Mar 24, 2023
@spastorino
Copy link
Member

Closing in favor of #109499

@spastorino spastorino closed this Mar 28, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 30, 2023
…-errors

Give return-position impl traits in trait a (synthetic) name to avoid name collisions with new lowering strategy

The only needed commit from this PR is the last one.

r? `@compiler-errors`

Needs rust-lang#109455.
@compiler-errors compiler-errors deleted the give-rpitits-real-names branch August 11, 2023 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants